Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix some tests to use udp.PickPort() #1302

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Fix some tests to use udp.PickPort() #1302

merged 1 commit into from
Sep 30, 2021

Conversation

yuhan6665
Copy link
Contributor

@codecov-commenter
Copy link

Codecov Report

Merging #1302 (418b499) into master (deb9d08) will decrease coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
- Coverage   44.64%   44.40%   -0.24%     
==========================================
  Files         485      476       -9     
  Lines       29534    29435      -99     
==========================================
- Hits        13184    13072     -112     
- Misses      14945    14949       +4     
- Partials     1405     1414       +9
Impacted Files Coverage Δ
transport/internet/udp/dispatcher.go 64.76% <0.00%> (-12.39%) ⬇️
common/buf/reader.go 82.35% <0.00%> (-5.89%) ⬇️
testing/servers/tcp/tcp.go 81.81% <0.00%> (-5.46%) ⬇️
transport/internet/udp/hub.go 73.61% <0.00%> (-4.17%) ⬇️
proxy/freedom/freedom.go 45.00% <0.00%> (-4.00%) ⬇️
transport/internet/system_listener.go 50.00% <0.00%> (-4.00%) ⬇️
transport/internet/kcp/dialer.go 80.39% <0.00%> (-3.93%) ⬇️
testing/scenarios/common.go 70.68% <0.00%> (-3.45%) ⬇️
transport/internet/kcp/receiving.go 97.31% <0.00%> (-2.69%) ⬇️
transport/internet/quic/hub.go 72.00% <0.00%> (-2.67%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update deb9d08...418b499. Read the comment docs.

@yuhan6665
Copy link
Contributor Author

This check fail:
https://github.com/v2fly/v2ray-core/pull/1302/checks?check_run_id=3726781714#step:5:2727
is not the original issue but a different one. This one seems more complicated which I will work on the fix later ;)

xiaokangwang
xiaokangwang approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

@yuhan6665 did't describe the root cause of the issue being fixed. Based on the content of the pull request I have a educated guess of why this fix is needed.

In the previous version of the test code, when attempting to listen for a UDP socket, the port availability check check for TCP port instead of UDP port. This make it possible to select a UDP port that is unavailable.

This pull request is ready to be merged.

@yuhan6665
Copy link
Contributor Author

Thanks for the review!
I will try to improve my English description in the future ;)

@xiaokangwang xiaokangwang merged commit 9458a1a into v2fly:master Sep 30, 2021
xiaokangwang pushed a commit that referenced this pull request Sep 30, 2021
@yuhan6665 @xiaokangwang
Fix some tests to use udp.PickPort()
d84ead3 
(cherry picked from commit 9458a1a, modified by committer)

In the previous version of the test code, when attempting to listen for a UDP socket, the port availability check check for TCP port instead of UDP port. This make it possible to select a UDP port that is unavailable.

See Also: #1302
@yuhan6665 yuhan6665 deleted the fix-udp-test branch October 3, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaokangwang xiaokangwang xiaokangwang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuhan6665 @codecov-commenter @xiaokangwang